-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multi tenancy iq handlers #3118
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3118 +/- ##
==========================================
+ Coverage 79.22% 79.24% +0.01%
==========================================
Files 389 394 +5
Lines 31959 32139 +180
==========================================
+ Hits 25321 25467 +146
- Misses 6638 6672 +34
Continue to review full report at Codecov.
|
57fc23a
to
bdc1bc2
Compare
bdc1bc2
to
b48c3e9
Compare
…ts for mongoose_lazy_routing
7f5bc61
to
25cd9c8
Compare
not sure why iq handlers are registered asynchronously, but keeping that as is (in case that is done to resolve some deadlocks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some style comments, but good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and quite complicated at the same time... There are some gen_server
s introduced, which can always become bottlenecks. Please double-check that all the calls really need to be synchronized this way.
%% KV pairs from the OldExtra map will remain unchanged, only | ||
%% the new keys from Extra map will be added to the NewExtra map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%% KV pairs from the OldExtra map will remain unchanged, only | |
%% the new keys from Extra map will be added to the NewExtra map | |
%% KV pairs from the OldExtra map will remain unchanged, | |
%% only the new keys from Extra map will be added to the NewExtra map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it much more readable when all the lines of the block comment have more or less the same length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to follow the ideas in https://sembr.org/ which really help with reading the comments. It's mostly for markup languages, but it helps me a lot with comments. I don't see a benefit of that stray "only" at the end of the sentence as it can be interpreted differently without seeing the next line (as an adverb instead of a conjunction).
ejabberd_local:register_host(Domain), | ||
ok; | ||
false -> | ||
%% we should never get here, but it's ok to just ignore this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if this gets called multiple times (because of a bug), we would silently ignore this fact.
@@ -69,6 +74,59 @@ auth_domain_removal_is_triggered_on_hook(_Config) -> | |||
1 = rpc(mim(), meck, num_calls, [ejabberd_auth_dummy, remove_domain, Params]), | |||
rpc(mim(), meck, unload, [ejabberd_auth_dummy]). | |||
|
|||
test_routing(Config) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split into 3 test cases.
I've seen that ejabberd_local has now its ets table as protected, why can't we do the same for ejabberd_sm' ets table? 🤔 |
it is protected. see http://erlang.org/doc/man/ets.html#new-2 :
|
Ah, protected is the default, I see, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k
this PR introduces iq handling for dynamic domains. here is the list of all changes:
mongoose_iq_handlers
packet_handler
's extra field and extending it withhost_type
for dynamic domainsmongoose_lazy_routing
module that controls dynamic domains/subdomains routing and iq handlers registration.TODO: introduce unit testing for
mongoose_lazy_routing
module